Fix status code for multipart subscriptions #3610
Conversation
Reviewer's Guide by SourceryThis pull request improves the multipart subscriptions code by addressing the status code handling in the streaming response and adding a test case to verify the status code. It also includes a TODO comment for potential future refactoring. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @patrick91 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider addressing the TODO comment about renaming the function before merging. If a rename is planned, it might be better to do it in this PR.
- It might be helpful to add a brief comment explaining why the default 200 status code is being set when sub_response.status_code is None.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -71,3 +71,5 @@ async def test_multipart_subscription( | |||
| data = [d async for d in response.streaming_json()] | |||
|
|
|||
| assert data == [{"payload": {"data": {"echo": "Hello world"}}}] | |||
|
|
|||
| assert response.status_code == 200 | |||
There was a problem hiding this comment.
suggestion (testing): Consider using HTTP status constants for better readability and maintainability
Instead of using the hard-coded value 200, it's recommended to use the status.HTTP_200_OK constant from the fastapi or starlette library. This improves code readability and ensures consistency with the rest of the codebase.
| assert response.status_code == 200 | |
| from starlette import status | |
| assert response.status_code == status.HTTP_200_OK |
|
Thanks for adding the Here's a preview of the changelog: This release fixes an issue with the http multipart subscription where the We also took the opportunity to update the internals to better support Here's the tweet text: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3610 +/- ##
==========================================
+ Coverage 96.75% 96.79% +0.03%
==========================================
Files 517 517
Lines 33528 33530 +2
Branches 5575 5576 +1
==========================================
+ Hits 32441 32455 +14
+ Misses 848 847 -1
+ Partials 239 228 -11 |
CodSpeed Performance ReportMerging #3610 will not alter performanceComparing Summary
|
patrick91
changed the title
|
EDIT: that works already |
Summary by Sourcery
Improve the multipart subscription handling by ensuring a default HTTP 200 OK status code is used when not explicitly set, and update tests to verify this behavior.
Bug Fixes:
Tests: